Repository navigation
Conversation
The screenshot tool assigned `drives.list_volumes`. That was the panel's scan until it was rewritten around `scan_batches`, after which the assignment created a new unused attribute instead of overriding anything, and nothing said so. The real scan then ran on every render: whether real drive labels and free space reached the PNGs came down to which write landed last, since the network batch arrives seconds after the local one. Patch the funnel every scan goes through, exit non-zero if that name ever disappears too, and assert before each picture that only the invented volumes are on screen — a check that reads the panel rather than trusting a monkeypatch, so the next rewrite of the scan cannot silently undo it.
The failure being guarded against was a patch that silently stopped patching, so a guard with no test of its own is the same shape of risk. Covers both halves: that the stub replaces the function the panel actually calls and yields a single final batch, leaving nothing to race; and that a missing — or merely non-callable — target stops the run instead of handing the panel back to the machine. The check itself is tested against a leaked network-share label, against several, and against an empty panel, which is the state before the first batch arrives and must not read as a leak.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed the screenshot scan replacement and the pre-capture guard against the actual drive-panel path. No actionable defect found in this diff.
The stub now replaces the callable that _ScanTask consumes, refuses to continue if that callable disappears, and emits one final synthetic batch so a delayed network batch cannot overwrite the invented volumes. The guard is checked before both main-window captures, which matters because the watcher polls between captures.
Validation: 8 targeted tests passed on this head; the base/head diff passed git diff --check. No interactive screenshot session was launched for this review. The label-only guard is narrower than a complete comparison of displayed volume rows, but the current scan stub itself is correctly connected.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed f070dff against the drive panel's actual scan entry point and the screenshot capture sequence.
No new actionable finding. The replacement targets drives.scan_batches, rejects a missing or non-callable target, and supplies one final synthetic batch. The guard runs before both main-window captures. Its label comparison is an additional check; isolation primarily depends on replacing the scan callable, since labels alone do not prove that every displayed field is synthetic.
Validation: all 8 screenshot-tool tests passed on this head. The tool was imported with temporary configuration, and the tests used synthetic rows. No screenshots were generated or published during this review.
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
No additional actionable defect found in this incremental diff.
The drive stub is installed at the _ScanTask.run lookup site before MainWindow construction, so asynchronous scans use synthetic volumes. Reviewed the scan batches, temporary configuration, visible-label check and configured fake-root guards against the drive watcher and capture sequence. The already discussed label-only guard limitation is not a new finding.
Validation: the eight screenshot-tool tests, unchanged from this head, passed as part of a 151-test run on detached descendant #12; two startup-source tests also passed there. The original stub/guard paths are unchanged, but #12 has additional screenshot fixtures, so this was a guard-test replay rather than an exact-head screenshot run. Current-head CI succeeds and the diff check passes.
Limits: no interactive desktop capture or live-volume disclosure experiment. Guard checks establish only the fields they actually inspect.
Pinned scope: declared base f9d11e3717b3ed988ee3fbc50e4243f78e4c4e3f, merge base f9d11e3717b3ed988ee3fbc50e4243f78e4c4e3f, head cdc4bc33a31626f1592777c36e7ad45ed9634e05. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.
1 participant
Summary
tools/screenshots.pypatcheddrives.list_volumesto keep real volumes out of the README images. The panel's scan was later rewritten aroundscan_batches, so that assignment stopped overriding anything and just created an unused attribute — silently. The real scan has been running on every render since.Whether real drive labels and free space actually reached the committed PNGs came down to a race:
_rebuild(VOLUMES)forces the invented list in, but the scan delivers local drives first and network shares seconds later, overwriting it. I hit exactly that while regenerating — one render came out with my real volumes, including an SMB share's label and capacity.The currently committed images are clean; this is about the next person to run the tool.
What changed
drives.scan_batches— the funnel every scan goes through — and yield a single final batch, so there is no second delivery to race the first.SystemExitif that name is ever missing too, rather than quietly handing the panel back to the machine.assert_no_real_volumes()before each picture, reading the labels actually on screen. That is the part that survives the next rewrite of the scan; patching a function name did not.Committed images are untouched by this PR — it changes the tool, not the output.
Test plan
ruff check toolscleanpython tools/screenshots.py OUTrenders all three images, exit 0, only invented volumesdrives.scan_batchesdeleted the run aborts with the explanatory message instead of rendering